Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge Comments and Post Comments blocks #41807

Merged
merged 46 commits into from
Jul 22, 2022
Merged

Conversation

DAreRodz
Copy link
Contributor

@DAreRodz DAreRodz commented Jun 20, 2022

Tracking issue: #41451

What?

Merge the Comments block into the Post Comments block. This PR is WIP and continues the work in #40522.

The idea is to end up with a single block comprising both blocks:

  • Comments as the default behavior
  • Post Comments as a fallback

More info in #40521.

We could also take this opportunity to change the Comments block ID from core/comments-query-loop to core/comments and end up with something like this:

image

Why?

See #40522 (comment)

How?

The block differentiates between the "legacy" PHP mode and the editable mode using an attribute called legacy. That property is false by default, and it's only set to true when converting the old Post Comments into the Comments block. The block conversion is automatically done when you open the WP editor.

The block is either static or dynamic depending on the value of legacy (false or true, respectively).

Testing Instructions

  1. Check out this branch.
  2. With a block theme installed that uses the Post Comment block in its Single template, visit a post with comments.
  3. Ensure that the Post Comments block renders as usual.
  4. Move to the Site Editor and open the Single template.
  5. Check that the block has turned into a Comments block. It should show a placeholder and a warning message to switch the block to the editable version.
  6. Open the code editor and verify that the block is now <!-- wp:comments {"legacy":true} /-->. Close the code editor.
  7. Save changes (you would need to make minor changes, like reordering blocks).
  8. Revisit the previous post. It should look exactly as before (i.e., the PHP version).
  9. Go to the Site Editor again, and now click the switch button. Now, inner blocks should appear.
  10. Open the code editor. The block should not have the legacy attribute (it is false by default) and should contain rendered markup and inner blocks.
  11. Save and visit the post again. The new version should render.

Other things to test:

  • Post Comments should not appear in the block inserter.
  • Adding the Comments block should show the editable version by default.

@DAreRodz DAreRodz marked this pull request as draft June 20, 2022 08:39
@DAreRodz
Copy link
Contributor Author

I thought the following code was working, but it turned out that I hadn't tested all the different possibilities.

const innerBlocksProps = useInnerBlocksProps.save( blockProps );
// The `save` function always returns a RawHTML element wrapping the
// serialized content. We check whether the serialized content is empty or
// not.
const rawHTML = Children.only( innerBlocksProps.children );
if ( ! rawHTML.props.children ) {
return null;
}
return <Tag { ...innerBlocksProps } />;

In line 15, rawHTML.props.children always return an empty string, whatever the inner blocks. It looks like the process that parses the block ignores its children, so this is not a valid approach to knowing whether the block has inner blocks or not. ☹️

$output = ob_get_clean();

wp_enqueue_script( 'comment-reply' );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
wp_reset_postdata();

@ockham
Copy link
Contributor

ockham commented Jun 20, 2022

I thought the following code was working, but it turned out that I hadn't tested all the different possibilities.

const innerBlocksProps = useInnerBlocksProps.save( blockProps );
// The `save` function always returns a RawHTML element wrapping the
// serialized content. We check whether the serialized content is empty or
// not.
const rawHTML = Children.only( innerBlocksProps.children );
if ( ! rawHTML.props.children ) {
return null;
}
return <Tag { ...innerBlocksProps } />;

In line 15, rawHTML.props.children always return an empty string, whatever the inner blocks. It looks like the process that parses the block ignores its children, so this is not a valid approach to knowing whether the block has inner blocks or not. ☹️

Sorry to hear -- I remember that it was one of the major problems with #40522 that we never found a straight-forward and reliable way to infer if the block has existing children or not.

I guess it's time we try a different strategy. At the time of conversion from post-comments or comments-query-loop to comments, we fortunately know which kind of block we're dealing with, so maybe our best option is to preserve that information as a block attribute after all. I.e.

<!-- wp:post-comments / -->    ➡️    <!-- wp:comments legacy="true" / -->

<!-- wp:comments-query-loop -->     <!-- wp:comments -->
    <!-- InnerBlocks / -->       ➡️      <!-- InnerBlocks / -->
<!-- /wp:comments-query-loop -->    <!-- /wp:comments -->

@ockham
Copy link
Contributor

ockham commented Jun 20, 2022

As discussed in Slack with @DAreRodz and @c4rl0sbr4v0, it might make sense to decouple the block merge from the ID change (to core/comments), for which we already have a PR (#40506).

I think we can continue working on both of them independently; we'll just need to merge #40506 before this one (and then rebase this one to pick up the changed IDs).

@ockham ockham added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jun 20, 2022
@ockham
Copy link
Contributor

ockham commented Jun 20, 2022

Another thing that @DAreRodz and I discussed on our video call earlier today is ensuring backwards compatibility.
Specifically, while we plan to add logic that will change core/post-comments and core/comments-query-loop blocks to core/comments in the editor, this will only affect those blocks when edited.

The question thus remains what happens if we have existing post content with those old blocks that haven't been edited.
We believe that:

  1. Since core/comments-query-loop has always been static, the serialized blocks should continue to work on the frontend (since it's just HTML, after all).
  2. For core/post-comments (which is dynamic), we can hopefully follow the precedent set by the PR that transformed core/query-loop into core/post-template. (This should allow us to drop the actual post-comments block folder and have it taken care of by the core/comments code.)

Comment on lines 21 to 28
variations: [
{
name: 'default',
isDefault: true,
innerBlocks: TEMPLATE,
scope: [ 'inserter' ],
},
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need the variations? I think we tried this approach to work around the issue with detecting inner blocks, but it looks like we'd be avoiding this issue now anyway if we go with the legacy attribute as discussed during our call 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @ockham. I'll remove the variations attribute. 👍

@luisherranz
Copy link
Member

In my opinion, whether a block contains inner blocks or not (during save) is a perfectly valid use case because a block may want to output different markup depending on that information.

So even if !!Children.only( useInnerBlockProps.save().children ).props.children would have worked, it should have been a temporary workaround, but that is not a good final API.

My suggestion is to investigate how to add this information to the Block API in a way that:

  • It works for both edit, save (and view/frontend if it's added in the future) in the same way, so code can be reused between those environments.
  • A similar API can also be exposed for dynamic blocks.
  • It is simple and intuitive, hopefully reusing similar concepts/APIs as the existing ones.
  • It can be augmented in the future (i.e., adding more information about the inner blocks, like their names or attributes).

If you do so, please add it to the Block API tracking issue #41236.

@DAreRodz
Copy link
Contributor Author

So even if !!Children.only( useInnerBlockProps.save().children ).props.children would have worked, it should have been a temporary workaround, but that is not a good final API.

Totally agree; that's even coupled with the useInnerBlockProps.save() implementation, so we would need to change this code the day .save() returns something different.

@DAreRodz
Copy link
Contributor Author

@spacedmonkey, sorry for the ping! I should have created this PR as a draft from the beginning. 😅 All the code you reviewed, I copy-pasted it from /packages/block-library/src/post-comments/index.php, which is basically the behavior we want to include as a fallback after merging the blocks.

I'll have in mind your suggestions, though. 😄

@spacedmonkey
Copy link
Member

which is basically the behavior we want to include as a fallback after merging the blocks.

Just because the code is copied from elsewhere, doesn't mean it should be just accepted into this new block. As the old block does seem to have unit tests, it hard to valid how it meant to work. I have 9 years of experience working on WordPress core and I am a core committer. Hopefully some of feedback is actioned.

@DAreRodz
Copy link
Contributor Author

@ockham, I'm not sure how to handle styles. Do we need to keep the old class name? Can we include styles only if the block version is the legacy one? 🤔

@ockham
Copy link
Contributor

ockham commented Jun 21, 2022

@ockham, I'm not sure how to handle styles. Do we need to keep the old class name? Can we include styles only if the block version is the legacy one? 🤔

Good question! In cases like these, I like to consult #32514 for prior art 😄

I found this, so I think we should keep the old class name.

(But yeah, in our case, we'd only need to keep the class name around if legacy is true.)

@DAreRodz
Copy link
Contributor Author

I have another question regarding styles, though. I'm not sure if this is feasible, but it would be ideal if the old CSS code for wp-block-post-comments is not included when the Comments block doesn't have the legacy attribute.

Is there a way to not include wp-block-post-comments in the block.json's style prop and request that stylesheet only when the block uses the legacy version, during the block rendering (in PHP)?

@ntsekouras
Copy link
Contributor

Is there a way to not include wp-block-post-comments in the block.json's style prop and request that stylesheet only when the block uses the legacy version, during the block rendering (in PHP)?

It seems there is not a way for that in _wp_multiple_block_styles and I'm not sure if we can use another filter for that.. --cc @aristath

I haven't checked the code in this PR but maybe an alternative could be to transform the legacy styles to css-in-js so they will be injected conditionally. --cc @ciampo

@aristath
Copy link
Member

I don't think it's possible to do it in the editor... However, on the frontend I think it should be possible to do it 🤔
The _wp_multiple_block_styles function is hooked in block_type_metadata, so what we can do is remove it from block.json, and then conditionally add it in the block_type_metadata hook if needed.

@adamziel adamziel removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jun 23, 2022
@adamziel
Copy link
Contributor

adamziel commented Jun 23, 2022

This is a significant change for a minor point release so I went ahead and removed this issue from the 6.0.1 editor tasks board and also removed the backport label @ockham. LMK if you disagree!

@ockham
Copy link
Contributor

ockham commented Jun 23, 2022

This is a significant change for a minor point release so I went ahead and removed this issue from the 6.0.1 editor tasks board and also removed the backport label @ockham. LMK if you disagree!

Yeah, it's kind of a big change 🤔 I think my main rationale for including it in 6.0.1 would be that the Comments block is still fairly fresh -- WP 6.0 was the first release that had it included, and that deprecated the Post Comments block. In a way, merging those two blocks is the logical continuation of that deprecation. We'd discussed it previously with @gziolo, who seemed in favor of including it in 6.0.1.

But yeah, I get your concerns; it wouldn't hurt to have a bit more time to test this... 🤔

@DAreRodz DAreRodz force-pushed the merge/comments-into-post-comments branch from e2c5688 to a79556e Compare July 7, 2022 13:16
lib/blocks.php Outdated Show resolved Hide resolved
@DAreRodz
Copy link
Contributor Author

I saw some unrelated e2e tests that seem to fail randomly. 😅 They didn't pass two commits ago, and now they did.

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for all your work on this @DAreRodz, and thanks for everyone who weighed in to makes this happen! 🙌

LGTM :shipit: 😎

@DAreRodz DAreRodz added the [Block] Comments Affects the Comments Block - formerly known as Comments Query Loop label Jul 22, 2022
@DAreRodz DAreRodz marked this pull request as ready for review July 22, 2022 12:26
@DAreRodz
Copy link
Contributor Author

I'm going to merge this PR, so it doesn't keep growing. 😅 🚀


As a follow-up, we can go deeper into @spacedmonkey suggestion of using wp_reset_postdata(). I've been reviewing that, and AFAIK everything works without that function because:

  1. comments_template() only works on 'single' pages (see this).
  2. postId value from context usually will match the current global $post variable, so nothing changes after running setup_postdata( $post_id ) (see this).

Another thing I've noticed is that the block renders in those cases where thecomments_template()'s output is empty. This should be a quick fix. 🙂

diff --git a/packages/block-library/src/comments/index.php b/packages/block-library/src/comments/index.php
index 5df2ed550a..ef8c98e391 100644
--- a/packages/block-library/src/comments/index.php
+++ b/packages/block-library/src/comments/index.php
@@ -62,6 +62,10 @@ function render_block_core_comments( $attributes, $content, $block ) {
 	$output = ob_get_clean();
 	$post   = $post_before;
 
+	if ( empty( $output ) ) {
+		return '';
+	}
+
 	$classnames = array();
 	// Adds the old class name for styles' backwards compatibility.
 	if ( isset( $attributes['legacy'] ) ) {

@ockham
Copy link
Contributor

ockham commented Jul 28, 2022

Thanks @DAreRodz!

Your reasoning about wp_reset_postdata() makes sense to me -- we can leave things as they are (unless we're missing something cc/ @spacedmonkey).

Another thing I've noticed is that the block renders in those cases where the comments_template()'s output is empty. This should be a quick fix. 🙂

I've filed #42737 for this, but was struggling to reproduce in what cases comments_template()'s output would be empty. (I tried a post with no comments and disabled comments for it to also hide the comments form, but we already have code in trunk to bail early in that case.) Maybe you can help me fill in the testing instructions there? 🙌

@DAreRodz
Copy link
Contributor Author

DAreRodz commented Aug 1, 2022

EDIT: moved my answer here.

bgardner added a commit to wpengine/frost that referenced this pull request Aug 3, 2022
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Comments Affects the Comments Block - formerly known as Comments Query Loop Needs User Documentation Needs new user documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants